[WIKI-804] fix: refactor image uploader #8210
Conversation
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on December 20. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
WalkthroughRefactors logo selector value handling; prevents duplicate uploads on mount in image uploader; consolidates file deletion detection into a single pass with an early-exit; expands icon typing for block menu; threads a new flaggedExtensions option through UtilityExtension and DropHandlerPlugin and updated drop/file-insert call sites; changes an asset restore endpoint parameter and updates SitesFileService restore URL and signature. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,mts,cts}📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
Files:
🧬 Code graph analysis (2)packages/services/src/file/sites-file.service.ts (3)
apps/api/plane/space/views/asset.py (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/editor/src/core/extensions/custom-image/components/uploader.tsx (1)
130-150: Verify guard ref is set for all code paths.The
hasTriedUploadingOnMountRefis set for the "drop" event (line 137) and when no meta exists (line 148), but not for the "insert" event path (lines 139-146). This could allow the effect to re-run for "insert" scenarios if the component re-mounts.While
hasOpenedFileInputOnceprovides some protection (line 140), consider settinghasTriedUploadingOnMountRef.current = trueafter line 145 for consistency:hasTriggeredFilePickerRef.current = true; imageComponentImageFileMap?.set(imageEntityId ?? "", { ...meta, hasOpenedFileInputOnce: true }); + hasTriedUploadingOnMountRef.current = true; } } else { hasTriedUploadingOnMountRef.current = true; }This ensures all code paths set the guard, preventing potential duplicate effects across all scenarios.
🧹 Nitpick comments (1)
packages/editor/src/core/plugins/file/delete.ts (1)
52-67: Consider usingfor...ofinstead offorEachwith async callback.The
forEachloop doesn't await the async callback, causing deletions to run in parallel without coordination. While error handling is present, this pattern can lead to unhandled promise rejections or race conditions.Apply this diff to properly await all deletions:
- removedFiles.forEach(async (node) => { + for (const node of removedFiles) { const nodeType = node.type.name as keyof NodeFileMapType; const src = node.attrs.src; const nodeFileSetDetails = NODE_FILE_MAP[nodeType]; if (!nodeFileSetDetails || !src) return; try { editor.storage[nodeType]?.[nodeFileSetDetails.fileSetName]?.set(src, true); // update assets list storage value editor.commands.updateAssetsList?.({ idToRemove: node.attrs.id, }); await deleteHandler(src); } catch (error) { console.error("Error deleting file via delete utility plugin:", error); } - }); + }Alternatively, if parallel execution is intentional, use
Promise.allSettledto properly track completion:- removedFiles.forEach(async (node) => { + await Promise.allSettled(removedFiles.map(async (node) => { const nodeType = node.type.name as keyof NodeFileMapType; const src = node.attrs.src; const nodeFileSetDetails = NODE_FILE_MAP[nodeType]; if (!nodeFileSetDetails || !src) return; try { editor.storage[nodeType]?.[nodeFileSetDetails.fileSetName]?.set(src, true); // update assets list storage value editor.commands.updateAssetsList?.({ idToRemove: node.attrs.id, }); await deleteHandler(src); } catch (error) { console.error("Error deleting file via delete utility plugin:", error); } - }); + }));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/editor/src/core/extensions/callout/logo-selector.tsx(1 hunks)packages/editor/src/core/extensions/custom-image/components/uploader.tsx(3 hunks)packages/editor/src/core/plugins/file/delete.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,mts,cts}
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.{ts,tsx,mts,cts}: Useconsttype parameters for more precise literal inference in TypeScript 5.0+
Use thesatisfiesoperator to validate types without widening them
Leverage inferred type predicates to reduce the need for explicitisreturn types in filter/check functions
UseNoInfer<T>utility to block inference for specific type arguments when they should be determined by other arguments
Utilize narrowing inswitch(true)blocks for control flow analysis (TypeScript 5.3+)
Rely on narrowing from direct boolean comparisons for type guards
Trust preserved narrowing in closures when variables aren't modified after the check (TypeScript 5.4+)
Use constant indices to narrow object/array properties (TypeScript 5.5+)
Use standard ECMAScript decorators (Stage 3) instead of legacyexperimentalDecorators
Useusingdeclarations for explicit resource management with Disposable pattern instead of manual cleanup (TypeScript 5.2+)
Usewith { type: "json" }for import attributes; avoid deprecatedassertsyntax (TypeScript 5.3/5.8+)
Useimport typeexplicitly when importing types to ensure they are erased during compilation, respectingverbatimModuleSyntaxflag
Use.ts,.mts,.ctsextensions inimport typestatements (TypeScript 5.2+)
Useimport type { Type } from "mod" with { "resolution-mode": "import" }for specific module resolution contexts (TypeScript 5.3+)
Use new iterator methods (map, filter, etc.) if targeting modern environments (TypeScript 5.6+)
Utilize newSetmethods likeunion,intersection, etc., when available (TypeScript 5.5+)
UseObject.groupBy/Map.groupBystandard methods for grouping instead of external libraries (TypeScript 5.4+)
UsePromise.withResolvers()for creating promises with exposed resolve/reject functions (TypeScript 5.7+)
Use copying array methods (toSorted,toSpliced,with) for immutable array operations (TypeScript 5.2+)
Avoid accessing instance fields viasuperin classes (TypeScript 5....
Files:
packages/editor/src/core/extensions/callout/logo-selector.tsxpackages/editor/src/core/extensions/custom-image/components/uploader.tsxpackages/editor/src/core/plugins/file/delete.ts
🧬 Code graph analysis (1)
packages/editor/src/core/extensions/custom-image/components/uploader.tsx (1)
packages/services/src/file/file-upload.service.ts (1)
uploadFile(24-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
packages/editor/src/core/extensions/callout/logo-selector.tsx (2)
54-67: LGTM: Clean extraction of emoji value.Extracting the type-casted value to a local constant eliminates duplication and improves readability while maintaining type safety through the conditional guard.
68-81: LGTM: Clean extraction of icon value.Extracting the type-casted value to a local constant eliminates duplication and improves readability while maintaining type safety through the conditional guard.
packages/editor/src/core/plugins/file/delete.ts (2)
24-24: LGTM: Efficient early exit guard.The skip check prevents unnecessary deletion processing when the SKIP_FILE_DELETION meta flag is set, improving performance and clarity.
38-50: LGTM: Efficient consolidation of removal detection.Collecting removed files in a single pass over the old state is more efficient than the previous per-transaction approach and improves code clarity.
packages/editor/src/core/extensions/custom-image/components/uploader.tsx (1)
43-43: LGTM: Guard ref prevents duplicate mount uploads.The
hasTriedUploadingOnMountReffollows React best practices for preventing duplicate effects in strict mode or during re-mounts.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/editor/src/core/components/menus/block-menu.tsx(2 hunks)packages/editor/src/core/extensions/extensions.ts(1 hunks)packages/editor/src/core/extensions/utility.ts(2 hunks)packages/editor/src/core/plugins/drop.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,mts,cts}
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.{ts,tsx,mts,cts}: Useconsttype parameters for more precise literal inference in TypeScript 5.0+
Use thesatisfiesoperator to validate types without widening them
Leverage inferred type predicates to reduce the need for explicitisreturn types in filter/check functions
UseNoInfer<T>utility to block inference for specific type arguments when they should be determined by other arguments
Utilize narrowing inswitch(true)blocks for control flow analysis (TypeScript 5.3+)
Rely on narrowing from direct boolean comparisons for type guards
Trust preserved narrowing in closures when variables aren't modified after the check (TypeScript 5.4+)
Use constant indices to narrow object/array properties (TypeScript 5.5+)
Use standard ECMAScript decorators (Stage 3) instead of legacyexperimentalDecorators
Useusingdeclarations for explicit resource management with Disposable pattern instead of manual cleanup (TypeScript 5.2+)
Usewith { type: "json" }for import attributes; avoid deprecatedassertsyntax (TypeScript 5.3/5.8+)
Useimport typeexplicitly when importing types to ensure they are erased during compilation, respectingverbatimModuleSyntaxflag
Use.ts,.mts,.ctsextensions inimport typestatements (TypeScript 5.2+)
Useimport type { Type } from "mod" with { "resolution-mode": "import" }for specific module resolution contexts (TypeScript 5.3+)
Use new iterator methods (map, filter, etc.) if targeting modern environments (TypeScript 5.6+)
Utilize newSetmethods likeunion,intersection, etc., when available (TypeScript 5.5+)
UseObject.groupBy/Map.groupBystandard methods for grouping instead of external libraries (TypeScript 5.4+)
UsePromise.withResolvers()for creating promises with exposed resolve/reject functions (TypeScript 5.7+)
Use copying array methods (toSorted,toSpliced,with) for immutable array operations (TypeScript 5.2+)
Avoid accessing instance fields viasuperin classes (TypeScript 5....
Files:
packages/editor/src/core/plugins/drop.tspackages/editor/src/core/extensions/utility.tspackages/editor/src/core/components/menus/block-menu.tsxpackages/editor/src/core/extensions/extensions.ts
🧠 Learnings (2)
📚 Learning: 2025-11-25T10:18:05.172Z
Learnt from: CR
Repo: makeplane/plane PR: 0
File: .github/instructions/typescript.instructions.md:0-0
Timestamp: 2025-11-25T10:18:05.172Z
Learning: Applies to **/*.{ts,tsx,mts,cts} : Use `.ts`, `.mts`, `.cts` extensions in `import type` statements (TypeScript 5.2+)
Applied to files:
packages/editor/src/core/extensions/utility.ts
📚 Learning: 2025-10-09T20:42:31.843Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7922
File: apps/admin/app/(all)/(dashboard)/ai/form.tsx:19-19
Timestamp: 2025-10-09T20:42:31.843Z
Learning: In the makeplane/plane repository, React types are globally available through TypeScript configuration. Type annotations like React.FC, React.ReactNode, etc. can be used without explicitly importing the React namespace. The codebase uses the modern JSX transform, so React imports are not required for JSX or type references.
Applied to files:
packages/editor/src/core/components/menus/block-menu.tsx
🧬 Code graph analysis (1)
packages/editor/src/core/extensions/utility.ts (2)
packages/editor/src/core/types/editor.ts (1)
IEditorProps(142-171)packages/editor/src/core/types/config.ts (1)
TFileHandler(5-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
packages/editor/src/core/components/menus/block-menu.tsx (2)
15-15: LGTM!The import correctly uses
import typefor the type-only import, following TypeScript best practices.
31-31: No issues found. TheISvgIconsinterface properly extendsReact.SVGAttributes<SVGElement>and explicitly includesclassName?: string | undefined, making the type expansion at line 31 fully compatible with the usage at line 241.packages/editor/src/core/extensions/extensions.ts (1)
41-56: flaggedExtensions is correctly threaded through CoreEditorExtensionsThe addition of
flaggedExtensionstoTArgumentsand its propagation intoUtilityExtension(andCoreEditorAdditionalExtensions) looks consistent withIEditorPropsand keeps the core extension wiring cohesive. No functional issues spotted here; just ensure all call sites constructingCoreEditorExtensionsnow reliably provideflaggedExtensionsalongsidedisabledExtensionsso the downstream plugins see a coherent view of extension state.If you want, you can run a quick search to confirm all
CoreEditorExtensions(invocations now passflaggedExtensionsfromIEditorProps/editor props. A simplerg -n "CoreEditorExtensions\(" -C3in the repo should help verify this.Also applies to: 58-72, 116-123
packages/editor/src/core/extensions/utility.ts (1)
54-58: Good centralization of flaggedExtensions in UtilityExtensionUsing
Pick<IEditorProps, "disabledExtensions" | "flaggedExtensions" | "getEditorMetaData">forPropsand forwardingflaggedExtensionsintoDropHandlerPluginis a clean way to keep the drop/paste pipeline in sync with the editor’s top‑level configuration. This wiring looks correct and matches the intended API surface; the remaining work is just to ensureDropHandlerPlugin/insertFilesSafelyactually make use offlaggedExtensionsas designed (see comments indrop.ts).If you later evolve the semantics of
flaggedExtensions, this is the right choke point to add any additional plugins or behaviors that depend on it; just double‑check that all editor entry points constructingUtilityExtensionpass consistentflaggedExtensionsfromIEditorProps.Also applies to: 60-62, 80-84
| type Props = { | ||
| disabledExtensions?: TExtensions[]; | ||
| flaggedExtensions?: TExtensions[]; | ||
| editor: Editor; | ||
| }; | ||
|
|
||
| export const DropHandlerPlugin = (props: Props): Plugin => { | ||
| const { disabledExtensions, editor } = props; | ||
| const { disabledExtensions, flaggedExtensions, editor } = props; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/editor/src/core/plugins/drop.tsRepository: makeplane/plane
Length of output: 4973
🏁 Script executed:
# Search for all usages of flaggedExtensions in the file
rg "flaggedExtensions" packages/editor/src/core/plugins/drop.ts -A 2 -B 2Repository: makeplane/plane
Length of output: 636
flaggedExtensions is passed to handlePaste but missing from handleDrop, and unused in insertFilesSafely
flaggedExtensions is properly passed to insertFilesSafely in handlePaste (line 37) but missing from the handleDrop call (lines 70–76). Additionally, flaggedExtensions is defined in InsertFilesSafelyArgs but never destructured or used inside insertFilesSafely (line 98 only destructures disabledExtensions, not flaggedExtensions), making the new parameter dead code.
This creates an inconsistency: when behavior is added to insertFilesSafely that depends on flaggedExtensions, it will only apply to paste events, not drag-and-drop events. Add flaggedExtensions to the handleDrop call to match handlePaste, and ensure insertFilesSafely actually uses it once the logic is implemented.
🤖 Prompt for AI Agents
In packages/editor/src/core/plugins/drop.ts around lines 8 to 16 and related
spots (handlePaste at ~line 37, handleDrop at ~lines 70–76, and
insertFilesSafely definition at ~line 98), flaggedExtensions is passed to
handlePaste but omitted from handleDrop and never destructured/used in
insertFilesSafely; update the handleDrop call to include flaggedExtensions
alongside disabledExtensions and editor so both paste and drop flows receive the
same args, and modify insertFilesSafely's parameter destructuring to include
flaggedExtensions (and wire it into the function body or placeholder where the
future logic will use it) so the parameter is not dead code and both paste and
drop behaviors remain consistent.
* fix: refactor uploader * fix: props * fix: sites fix
* fix: refactor uploader * fix: props * fix: sites fix
* fix: refactor uploader * fix: props * fix: sites fix
Description
Refactored the entire image uploader flow to make it more reliable and easier to maintain. The changes clean up old upload logic, remove duplicated handlers, and unify how images are processed across create/edit views. This also reduces unnecessary re-renders and fixes edge-cases where images wouldn’t properly update after replacement.
Type of Change
Screenshots and Media (if applicable)
N/A
Test Scenarios
References
Improves overall reliability of the image upload experience.
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.